Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sockets - Cyndi #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sockets - Cyndi #29

wants to merge 1 commit into from

Conversation

cyndilopez
Copy link

No description provided.

@kaseea
Copy link

kaseea commented Mar 4, 2019

very clean looking! I think you missed the case for nil and empty array though.

Copy link

@eric-andeen eric-andeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Eric. I'll be your volunteer code reviewer. I've been a software developer for about a zillion years. I'll be applying most of the same professional standards to your code as I do in the day job. Some of my comments may seem pedantic, persnickety, or irrelevant to the kinds of exercises you're doing in class, but once you start your internship, you'll get the same kind of feedback I'm giving you here.

Good code for your first assignment.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
Copy link

@eric-andeen eric-andeen Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter names are inconsistent: arr_1 and array_2.
Parameter names are also unnecessarily verbose. array1 and array2 (as in the problem statement) would be fine.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
arr_integers_1.length.times do |index|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle the cases of array1 == nil, array2 == nil, array1.length != array2.length. This statement DOES cover zero-length arrays.

@@ -0,0 +1,8 @@
def array_equals(arr_integers_1, array_integers_2)
arr_integers_1.length.times do |index|
return false if arr_integers_1[index] - array_integers_2[index] != 0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a != b is much clearer than a - b != 0, even if mathematically equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants